-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Tag ignored tests that require SubqueryBroadcastExec #647
Conversation
@kazuyukitanimura please take a look |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #647 +/- ##
=============================================
+ Coverage 33.42% 53.81% +20.39%
- Complexity 805 811 +6
=============================================
Files 109 106 -3
Lines 42462 10245 -32217
Branches 9342 1917 -7425
=============================================
- Hits 14191 5513 -8678
+ Misses 25322 3755 -21567
+ Partials 2949 977 -1972 ☔ View full report in Codecov by Sentry. |
@@ -442,7 +442,7 @@ index 2c24cc7d570..50a2ce86117 100644 | |||
|
|||
- test("partition pruning in broadcast hash joins with aliases") { | |||
+ test("partition pruning in broadcast hash joins with aliases", | |||
+ IgnoreComet("TODO: https://github.com/apache/datafusion-comet/issues/551")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, aren't they already ignored? Seems this patch only changes the ignore reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant before this PR, the diffs already have IgnoreComet
which ignores these tests. Isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct, these tests are ignored currently, so no change in test coverage. The purpose of this PR is to tag them with #242 so that they will be fixed all together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, thanks for confirming it. The PR title seems inaccurate so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @parthchandra Would you like to update the title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the commit and PR title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @parthchandra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merged. Thanks @parthchandra @kazuyukitanimura |
Which issue does this PR close?
Addresses part of #551
Rationale for this change
This addresses the following tests which require SubqueyBroadcastExec to be implemented